Fix invisible RTL text on Qt backend (MeasureWidths uses visual instead of logical positions) - #1116
Closed
NoamRamadi wants to merge 1 commit into
Closed
Fix invisible RTL text on Qt backend (MeasureWidths uses visual instead of logical positions)#1116NoamRamadi wants to merge 1 commit into
NoamRamadi wants to merge 1 commit into
Conversation
Scintilla's rendering code (EditView.cxx) requires Surface::MeasureWidths() to return per-character pixel positions that are monotonically non-decreasing in logical (document) order, so it can place styled runs, the caret, and selection highlights along a line. The Win32 and Cocoa backends already honor this by summing cluster advance widths in text order. The Qt backend instead used QTextLine::cursorToX(), which returns visual (bidi-reordered) positions. For a line containing only RTL text (e.g. Hebrew), Qt auto-detects the paragraph as right-to-left and cursorToX() returns positions that decrease as the logical index increases, breaking the monotonicity invariant. This pushes the line's glyphs outside the visible/ clipped drawing rectangle, making RTL-only lines appear invisible, while mixed LTR+RTL lines (e.g. "Hi היי") happened to stay partially in bounds. Fix MeasureWidths/MeasureWidthsUTF8 in PlatQt.cpp to accumulate each character's advance width independently via QFontMetricsF::horizontalAdvance, summed in logical order, guaranteeing the required monotonic invariant regardless of text direction. Glyph painting already goes through QPainter::drawText, which performs correct bidi shaping/reordering on its own, so visual rendering quality is unaffected.
Author
|
Note on the failing checks: both the Codespell failure (a pre-existing `alot` typo in `src/dialogs/MainWindow.cpp`, a file this PR doesn't touch) and the `macos-latest` build failures for Qt 6.5/6.8 (`ld: framework 'AGL' not found`) already reproduce identically on `master` at c627dea (see the latest "Build Notepad Next" / "Codespell" runs on master), so they're unrelated to this change. All Linux and Windows builds pass, and macOS with Qt 6.10 passes. |
Owner
|
The Scintilla code here is just a direct copy of the upstream https://scintilla.org/ project. Any bug fixes would need submitted to the upstream project to review/accept. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RTL-only lines (e.g. typing Hebrew alone on a line) render invisibly. Mixed LTR+RTL lines (e.g.
Hi היי) render correctly, which pointed at a positioning bug rather than a font/shaping issue.Root cause:
Surface::MeasureWidths()is expected to return per-character pixel positions that are monotonically non-decreasing in logical (document) order — Scintilla'sEditView.cxxuses this to place styled runs, the caret, and selection highlights along a line. The Win32 (SurfaceD2D::MeasurePositions) and Cocoa backends already provide this by summing cluster advance widths in text order, independent of visual/bidi reordering.The Qt backend's
SurfaceImpl::MeasureWidths/MeasureWidthsUTF8(PlatQt.cpp) instead usedQTextLine::cursorToX(), which returns visual bidi-reordered positions. For a line containing only RTL characters, Qt auto-detects the paragraph as right-to-left, andcursorToX()returns positions that decrease as the logical index increases — violating the monotonicity invariantEditView.cxxdepends on. This pushes glyphs for the line outside the visible/clipped drawing rectangle, making pure-RTL lines appear invisible. A leading LTR run (as inHi היי) happens to anchor enough of the line in-bounds to look correct.Fix
Changed
MeasureWidths(UTF-8 codepage branch) andMeasureWidthsUTF8inthirdparty/scintilla/qt/ScintillaEditBase/PlatQt.cppto accumulate each character's advance width independently viaQFontMetricsF::horizontalAdvance(), summed in logical order — guaranteeing the required monotonic-non-decreasing invariant regardless of text direction, matching the approach already used on Win32/Cocoa.Glyph painting (
DrawTextNoClip/DrawTextClipped, etc.) already goes throughQPainter::drawText(), which performs correct Unicode bidi shaping/reordering on its own — so visual rendering quality for LTR text and RTL glyph shaping is unaffected by this change.Known trade-off: per-character measurement loses cross-character contextual shaping (ligatures, ligature-based kerning, Arabic's position-dependent glyph forms) for measurement purposes only. Hebrew (the reported case) has minimal contextual shaping, so this is not noticeable. Arabic text should no longer be invisible, though pixel-perfect character spacing for heavily-shaped scripts could still have minor rounding differences from what
QPainter::drawTextactually paints — a possible follow-up if anyone hits it.Out of scope for this PR: implementing
SurfaceImpl::Layout()/IScreenLineLayoutand wiring upMessage::SetBidirectional(currently unimplemented on the Qt backend, unlike Win32/Cocoa). That's needed for fully correct RTL caret movement and multi-rectangle RTL selection highlighting, but is a separate, larger feature addition — not required to fix the reported invisible-text bug, sinceEditModel::BidirectionalEnabled()is never true today on Qt builds (nothing callsSetBidirectional).Test plan
cmake -S . -B build -DCMAKE_PREFIX_PATH=$(brew --prefix qt) -DCMAKE_BUILD_TYPE=Debug && cmake --build build --target NotepadNext -jהייalone on a line — now visible (previously invisible)Hi הייon a line — still renders correctly (no regression)🤖 Generated with Claude Code